Adding try_state hook(tips & vesting)
#13515
Conversation
|
Would be good if you update the PR title to say which pallets it includes. |
Doordashcon
changed the title
try_state hooktry_state hook(vesting, tips & sudo)
Doordashcon
changed the title
try_state hook(vesting, tips & sudo)try_state hook(vesting & tips)
Doordashcon
changed the title
try_state hook(vesting & tips)try_state hook(tips & vesting)
|
I have ran the try-state expectations of Tips and Vesting pallet against the current Polkadot state, please review :) |
|
|
frame/vesting/src/mock.rs
Outdated
| ext.execute_with(|| { | ||
| System::set_block_number(1); | ||
| Vesting::do_try_state().unwrap(); | ||
| }); |
There was a problem hiding this comment.
I think this doesn't really test much because it runs before there's any state set up to test?
I suggest that you
- test that the try-state hooks will detect the issues that they're supposed to, check out how that's done here: https://github.com/paritytech/substrate/blob/master/frame/bags-list/src/list/tests.rs#L353-L378 (also do this with
tips) - pepper
assert_ok!(vesting::do_try_state());s into existing tests, like you did withtips
There was a problem hiding this comment.
But i still think it's all the same and there's no need to pepper existing tests, the current setup in vesting tests the genesis state.
There was a problem hiding this comment.
Let me know if I'm missing something, but I think we should test all functionality not just the genesis state?
Like bags-list there should be tests ensuring that the try-state hooks actually detect the issues that they're supposed to: https://github.com/paritytech/substrate/blob/master/frame/bags-list/src/list/tests.rs#L353-L378
There was a problem hiding this comment.
Hello @liamaharon , I have made updates to this. Please review
There was a problem hiding this comment.
Please add tests ensuring that the try-state hooks detects the issues they are intended to. They’d have picked up a bug in the assert (see other comment)
frame/vesting/src/lib.rs
Outdated
| let mut total_per_block: BalanceOf<T> = Zero::zero(); | ||
| let mut total_locked_now: BalanceOf<T> = Zero::zero(); | ||
| for info in vesting.iter() { | ||
| // get the number of remaining vesting blocks<>balance |
There was a problem hiding this comment.
I don't understand this comment
There was a problem hiding this comment.
the number of blocks remaining are converted to balance
There was a problem hiding this comment.
Please update this comment with this clarification
liamaharon
added
B0-silent
There was a problem hiding this comment.
I don't mean to sound harsh, but this PR was not ready to be re-reviewed.
The core logic of the vesting try-state hook is broken, there’s an ignored suggestion from the last review which would have uncovered that issue, and comments had not been updated to reflect code changes.
Please only request review after you have addressed comments from the previous review and thoroughly checked your logic, code and comments.
I suggest that you step back from the code for a minute, think about the invariant/s you are testing, and thoughtfully plan the logic that that will test these invariants, writing it down in a plain English doc comment. Then as you codify that logic, write tests to ensure that the code is working as you intended, detecting failure cases as well as letting through success cases.
frame/vesting/src/lib.rs
Outdated
| let one_extra = total_to_vest.saturating_sub(total_locked); | ||
| assert_eq!(total_to_vest, total_locked.saturating_add(one_extra)); |
There was a problem hiding this comment.
So what this is checking is:
if one_extra = total_to_vest - total_locked does total_to_vest = one_extra + total_locked?
or more abstractly
if x = y - z, then does y = x + z?
of course it will always be true, you're asserting that 1 == 1.
frame/vesting/src/lib.rs
Outdated
| let mut total_per_block: BalanceOf<T> = Zero::zero(); | ||
| let mut total_locked_now: BalanceOf<T> = Zero::zero(); | ||
| for info in vesting.iter() { | ||
| // get the number of remaining vesting blocks<>balance |
There was a problem hiding this comment.
Please update this comment with this clarification
frame/vesting/src/lib.rs
Outdated
| let amount_left: BalanceOf<T> = | ||
| info.ending_block_as_balance::<T::BlockNumberToBalance>(); | ||
| total_to_vest += info.per_block().saturating_mul(amount_left); | ||
| // get the total block<>balance still locked. |
There was a problem hiding this comment.
Could you please replace this block<>balance shorthand throughout the comments with just plain English
frame/vesting/src/lib.rs
Outdated
| /// * The `per_block` amount left to be claimed is equal to the | ||
| /// total balance currently locked. |
There was a problem hiding this comment.
Outdated variable name in comment
frame/vesting/src/lib.rs
Outdated
| /// *`total_to_vest` the total amount left to vest over all remaining blocks. | ||
| /// *`total_locked` the total amount locked. | ||
| /// | ||
| /// * `one_extra` accounts for the extra block that | ||
| /// will be added to the vesting schedule if `per_block` is bigger than `locked`. |
There was a problem hiding this comment.
Please add documentation for variables the line above where the variable is declared, rather than in the doc comment
frame/vesting/src/mock.rs
Outdated
| ext.execute_with(|| { | ||
| System::set_block_number(1); | ||
| Vesting::do_try_state().unwrap(); | ||
| }); |
There was a problem hiding this comment.
Please add tests ensuring that the try-state hooks detects the issues they are intended to. They’d have picked up a bug in the assert (see other comment)
|
Hello @liamaharon, if you could spare some time... I am currently trying to denote where the logic is that updates the Time(moment) passes and now we are at block 11, This is the logic I am searching for, the one that knows to execute the update. |
part of paritytech/polkadot-sdk#239
polkadot address: 12zsKEDVcHpKEWb99iFt3xrTCQQXZMu477nJQsTBBrof5k2h